Refactor the gateway integration test suite into a dedicated ros2_medkit_integration_tests package#227
Refactor the gateway integration test suite into a dedicated ros2_medkit_integration_tests package#227
ros2_medkit_integration_tests package#227Conversation
There was a problem hiding this comment.
Pull request overview
This pull request extracts integration tests and demo nodes from ros2_medkit_gateway into a new dedicated package ros2_medkit_integration_tests. The refactoring improves test organization by separating integration tests into two categories: scenario tests (end-to-end stories) and feature tests (atomic, independent tests). A new ros2_medkit_test_utils Python package provides shared utilities including launch helpers, base test classes, and constants.
Changes:
- Created new
ros2_medkit_integration_testspackage with demo nodes, test utilities, and reorganized tests - Moved 9 demo node C++ files from
ros2_medkit_gateway/test/demo_nodes/to the new package - Split monolithic integration tests into 23 smaller test files organized by scenario vs. feature
- Removed integration test dependencies from
ros2_medkit_gatewaypackage.xml and CMakeLists.txt - Updated launch files and test references to use new package names
Reviewed changes
Copilot reviewed 39 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_integration_tests/package.xml | New package metadata with test and demo node dependencies |
| src/ros2_medkit_integration_tests/CMakeLists.txt | Build config for demo nodes and test registration with proper timeouts |
| src/ros2_medkit_integration_tests/setup.cfg | Python package setup for test utilities |
| src/ros2_medkit_integration_tests/ros2_medkit_test_utils/* | Shared test utilities: constants, coverage helpers, launch factories, base test class |
| src/ros2_medkit_integration_tests/test/scenarios/* | 9 scenario test files (end-to-end stories, 300s timeout) |
| src/ros2_medkit_integration_tests/test/features/* | 14 feature test files (atomic tests, 120s timeout) |
| src/ros2_medkit_integration_tests/demo_nodes/* | 9 demo node C++ files moved from gateway package |
| src/ros2_medkit_integration_tests/launch/demo_nodes.launch.py | Launch file updated to reference new package |
| src/ros2_medkit_gateway/package.xml | Removed test dependencies (pytest, launch_testing, etc.) |
| src/ros2_medkit_gateway/CMakeLists.txt | Removed integration tests and demo node build targets |
ros2_medkit_integration_tests package
ae470cf to
8050710
Compare
8050710 to
86cd8f4
Compare
Four modules replacing ~1200 lines of duplicated code: - constants.py: API paths, ports, timeouts - coverage.py: unified get_coverage_env() - launch_helpers.py: factory for gateway/demo/fault_manager nodes - gateway_test_case.py: base class with health polling, HTTP helpers, wait helpers, and discovery assertions Refs #139, #222
Replaces test_discovery_manifest and test_discovery_hybrid from gateway package with scenario-style tests using shared assertion helpers. Each scenario validates entity structure, capabilities, and mode-specific behavior. Refs #139
Migrated from gateway package with scenario-style structure. Using shared test_utils for launch helpers and base test case. Refs #139
- Add 'calibration_service' and 'long_calibration_action' aliases to DEMO_NODE_REGISTRY (fixes KeyError in test_entity_routing and test_operations_api) - Fix camelCase 'faultCode' -> snake_case 'fault_code' in test_scenario_fault_lifecycle to match actual API response - Replace strict assertExitCodes with SIGTERM-tolerant exit code check across all 24 test files (exit code -15 is expected during launch_testing shutdown) - Add polling loop in test_scenario_discovery_hybrid test_15 to wait for runtime linking (apps become online asynchronously after nodes start)
- long_calibration_action: replace detached thread with joinable thread and atomic shutdown flag; add try-catch for goal handle interactions; use set_terminate to handle rclcpp_action race during SIGINT shutdown - All timer-based demo nodes: cancel timers in destructor to prevent callbacks firing during node destruction (fixes SIGSEGV on Humble) - Revert gateway-only exit code guard: all processes must exit cleanly - Fix test_discovery_heuristic exit code check (was silently passing)
…n tests package - Add README.md with package structure, test templates, and GatewayTestCase API - Add design/index.rst with PlantUML architecture diagram and test catalog - Add symlink and toctree entry in docs/design/ - Add package description in docs/introduction.rst - Fix demo_nodes.launch.py package reference in getting_started.rst - Update devcontainer.rst test filtering for new package structure
- Add REQUIRED_APPS to test_hateoas discovery wait so temp_sensor is guaranteed to be discovered before tests run (fixes 404 on all platforms) - Increase fault_manager test timeout from 1s to 3s for Humble's slower DDS service discovery - Accept either "not available" or "timed out" error in service availability tests (wait_for_service behavior varies across distros) - Exclude vendored/ from coverage in both codecov.yml and CI lcov filter
On Humble (CycloneDDS), reusing the same node name across sequential GTest cases causes stale DDS participant discovery state. This corrupts service responses in the second test when the first test did not create a service. Using unique node names per test eliminates the collision. Fixes test_fault_manager on Humble CI (GetSnapshotsSuccessWithValidJson).
86cd8f4 to
f0d0c3f
Compare
… race Add poll_endpoint() and poll_endpoint_until() to the base test case, replacing 8 duplicated polling helpers across 7 test files. The predicate-based poll_endpoint_until() supports both boolean checks and value extraction from response JSON. Fixes test_app_no_topics flake on Jazzy where the calibration service-only node transiently disappeared from the ROS 2 graph between the entity readiness check and the data request.
| f'{self.LIDAR_ENDPOINT}/faults', | ||
| lambda d: next( | ||
| ( | ||
| item.get('faultCode') |
There was a problem hiding this comment.
Bug: The poll condition uses faultCode (camelCase) but the listing API returns fault_code (snake_case) - confirmed in fault_manager.cpp:61. This means the condition never matches, poll_endpoint_until times out, and all 6 tests in this file silently skip via skip_on_timeout=True.
# Current (never matches):
item.get('faultCode')
# Should be:
item.get('fault_code')| <depend>ros2_medkit_msgs</depend> | ||
|
|
||
| <!-- Test dependencies (Python integration tests) --> | ||
| <test_depend>launch_testing_ament_cmake</test_depend> |
There was a problem hiding this comment.
Missing a few test dependencies that could bite on minimal build environments:
<test_depend>launch_testing</test_depend>
<test_depend>launch_ros</test_depend>
<test_depend>ament_index_python</test_depend>launch_testing_ament_cmake is the CMake glue, but the Python packages (launch_testing, launch_ros) and ament_index_python (used in coverage.py) are separate.
There was a problem hiding this comment.
Fixed in e0e39e0 - added all three as <test_depend>.
| time.sleep(DISCOVERY_INTERVAL) | ||
|
|
||
| # Deadline reached -- warn but continue; individual tests may fail. | ||
| print('Warning: Discovery timeout, some tests may fail') |
There was a problem hiding this comment.
_wait_for_discovery() only prints a warning on timeout, while _wait_for_gateway_health() raises SkipTest. This asymmetry means a slow discovery causes a wall of confusing cascading failures instead of a clean skip. Would it make sense to align these?
# Current:
print('Warning: Discovery timeout, some tests may fail')
# Suggested:
raise unittest.SkipTest(
f'Discovery incomplete after {DISCOVERY_TIMEOUT}s — '
f'found {len(discovered_apps)} apps, need {cls.MIN_EXPECTED_APPS}'
)There was a problem hiding this comment.
Both _wait_for_gateway_health() and _wait_for_discovery() now raise AssertionError on timeout. Fixed in b12dbc4.
| def test_16_app_has_runtime_topics(self): | ||
| """Online app has topics from runtime discovery.""" | ||
| # Wait a bit for runtime linking | ||
| time.sleep(3) |
There was a problem hiding this comment.
These tests (16-17, 22-23) wrap assertions in if response.status_code == 200: guards, which means they pass silently when the endpoint isn't ready. Compare with test_15 which correctly uses poll_endpoint_until(). Could we do the same here?
# Current (silently passes without asserting anything):
response = requests.get(f'{self.BASE_URL}/apps/engine-temp-sensor/data', timeout=5)
if response.status_code == 200:
data = response.json()
if 'items' in data and data['items']:
# ...assert...
# Suggested — poll until data arrives, then assert unconditionally:
data = self.poll_endpoint_until(
'/apps/engine-temp-sensor/data',
lambda d: d.get('items'),
timeout=15.0,
)
topic_names = [t.get('name', '') for t in data]
self.assertTrue(any('temperature' in name for name in topic_names))Same pattern in test_scenario_discovery_manifest.test.py:364 (test_25_discovery_stats).
There was a problem hiding this comment.
Fixed in e0e39e0 - replaced if status == 200 guards with poll_endpoint_until() in the hybrid test.
| Online status depends on runtime linking timing. | ||
| """ | ||
| # Wait a bit for runtime linking | ||
| time.sleep(5) |
There was a problem hiding this comment.
Bare time.sleep(5) instead of polling. Flaky on slow CI, wasteful on fast machines. The hybrid test_15 already uses poll_endpoint_until() correctly - could this follow the same pattern?
Same issue in test_scenario_discovery_hybrid.test.py:298 (time.sleep(3)).
There was a problem hiding this comment.
Fixed in e0e39e0 - replaced bare time.sleep() with poll_endpoint_until() in manifest and hybrid tests.
| # Wait for data availability on the discovered app | ||
| # Skip ROS 2 system topics that don't have continuous data flow | ||
| system_topics = {'/parameter_events', '/rosout'} | ||
| deadline = time.time() + 15.0 |
There was a problem hiding this comment.
Minor: time.time() here while the base class consistently uses time.monotonic() (immune to NTP clock adjustments). Worth aligning for consistency.
There was a problem hiding this comment.
Fixed in b12dbc4 - all timing now uses time.monotonic().
| @@ -223,16 +181,15 @@ def test_01_create_subscription_returns_201_with_correct_schema(self): | |||
| self._delete_subscription(sub['id']) | |||
There was a problem hiding this comment.
If an assertion fails before _delete_subscription(), the subscription leaks and can affect subsequent tests. addCleanup() would guarantee cleanup regardless:
sub = self._create_subscription()
self.addCleanup(self._delete_subscription, sub['id'])(I noticed addCleanup / tearDown aren't used anywhere in the package currently.)
There was a problem hiding this comment.
Fixed in f554a46 - added addCleanup to test_12. All subscription-creating tests now use it.
|
|
||
| """ | ||
|
|
||
| PORT = DEFAULT_PORT |
There was a problem hiding this comment.
Nit: PORT class attribute is never read by any test (confirmed with grep). Subclasses override BASE_URL directly. Could just remove PORT to avoid confusion.
| self.assertIn('capabilities', data) | ||
|
|
||
| self.assertEqual(data['name'], 'ROS 2 Medkit Gateway') | ||
| self.assertEqual(data['version'], '0.1.0') |
There was a problem hiding this comment.
assertEqual(data['version'], '0.1.0') will break on every version bump. A format check (e.g. semver regex) would be more resilient.
There was a problem hiding this comment.
Fixed in e0e39e0 - uses semver regex (assertRegex) instead of exact match.
| self.assertEqual(verify_data['x-medkit']['parameter']['value'], 80.0) | ||
|
|
||
| # Reset the value back to default using SOVD "data" field | ||
| requests.put( |
There was a problem hiding this comment.
If the assertion fails before this reset PUT, min_temp stays at 80.0. addCleanup() would guarantee the reset regardless of test outcome.
There was a problem hiding this comment.
Fixed in f554a46 - added addCleanup with DELETE to reset min_temp to default.
|
Stale references The launch file moved but a few references still point to the old package:
All say |
| def setUpClass(cls): | ||
| """Wait for gateway to be ready and get tokens.""" | ||
| cls._wait_for_gateway_health() | ||
| time.sleep(1) |
There was a problem hiding this comment.
If the auth endpoint isn't ready, response.status_code != 200 and the token is never stored. Then _auth_header() returns Bearer "" and auth tests fail with misleading errors. Maybe add a check after the loop?
missing = [r for r in ('admin', 'operator', 'viewer', 'configurator') if r not in cls.tokens]
if missing:
raise unittest.SkipTest(f'Could not acquire tokens for: {", ".join(missing)}')There was a problem hiding this comment.
Fixed in b12dbc4 - added a check after the token acquisition loop.
…ility - Fix faultCode->fault_code bug that caused 6 fault tests to silently skip - Remove skip_on_timeout parameter from poll_endpoint/poll_endpoint_until; tests now fail on timeout instead of skipping - Replace silent if-status_code-200 guards with poll_endpoint_until and get_json assertions in hybrid/manifest discovery tests - Replace bare time.sleep with polling in manifest discovery test - Align _wait_for_discovery timeout to raise SkipTest (like health check) - Use time.monotonic instead of time.time in subscriptions test - Add addCleanup for subscription and config cleanup on assertion failure - Validate auth token acquisition, skip with clear message if tokens missing - Replace hardcoded version assert with semver regex - Remove unused PORT class attribute and DEFAULT_PORT import - Add missing launch_testing, launch_ros, ament_index_python test_depend - Fix stale demo_nodes.launch.py references to integration_tests package
Tests must fail, not skip. No hidden bugs. - gateway_test_case: _wait_for_gateway_health and _wait_for_discovery now raise AssertionError instead of SkipTest on timeout - auth test: missing tokens raise AssertionError instead of SkipTest - 21 self.skipTest() calls across 9 test files replaced with self.fail() (subscriptions, bulk_data_download, fault_inspection, entity_routing, operations_api, action_lifecycle, discovery_manifest, config_management, data_read, data_publish_verify)
The previous commit replaced skipTest() with fail(), exposing tests that never actually validated anything. This fixes the root causes: - fault_inspection test_02: fix assertions to match actual C++ status fields (aggregatedStatus, confirmedDTC, pendingDTC as strings) - fault_inspection test_04/05: add wait_for_fault_detail() shared helper that polls until async snapshots are captured; configure snapshots.default_topics for freeze_frame capture; set confirmation_threshold=-2 so rosbag ring buffer has time to collect data before first fault confirmation - data_read test_list_function_data: remove from runtime-mode test (functions require manifest; already covered in manifest test_23) - discovery_manifest test_18: accept empty data items in manifest_only mode (runtime graph discovery disabled, no topic enrichment) - discovery_manifest test_25: remove test for nonexistent /discovery/stats endpoint
- Add addCleanup to subscription test_12 and config test_delete to prevent resource leaks on assertion failures - Remove unused PORT docstring from GatewayTestCase - Fix wait_for_fault_detail to check all faults for required snapshot types (rosbag buffer is shared, only one fault gets the data when multiple confirm simultaneously) - Stabilize fault_lifecycle test with short rosbag durations (0.1s) and confirmation_threshold=-2 so captures complete before shutdown
|
|
||
| # === Python test utilities === | ||
|
|
||
| ament_python_install_package(ros2_medkit_test_utils) |
There was a problem hiding this comment.
Blocker: ament_python_install_package(ros2_medkit_test_utils) is called but there is no find_package(ament_cmake_python REQUIRED) (lines 11-19). Also missing <buildtool_depend>ament_cmake_python</buildtool_depend> in package.xml. Build will fail at cmake configure with "Unknown CMake command".
| items = data['items'] | ||
| self.assertIsInstance(items, list) | ||
|
|
||
| if len(items) > 0: |
There was a problem hiding this comment.
Major: Five if len(items) > 0: guards (lines 68, 90, 109, 128, 439) silently skip all structural assertions when items is empty. Tests pass with zero assertions. Use self.assertGreater(len(items), 0) before the loop or poll_endpoint_until with a non-empty condition.
|
|
||
| # Should return 200 for sync or 400/500 if service is unavailable | ||
| self.assertIn( | ||
| response.status_code, [200, 400, 500, 503], |
There was a problem hiding this comment.
Major: Accepts [200, 400, 500, 503]. HTTP 500 is a server bug, not a valid outcome. Remove 500 from the list.
|
|
||
| # Should return 200 for sync or 400/500/503 if service unavailable | ||
| self.assertIn( | ||
| response.status_code, [200, 400, 500, 503], |
There was a problem hiding this comment.
Major: Same issue: accepts [200, 400, 500, 503]. Remove 500.
| self.assertEqual(get_response.status_code, 200) | ||
| get_data = get_response.json() | ||
| # The value should be reset to default (85.0) | ||
| _ = get_data['x-medkit']['parameter']['value'] |
There was a problem hiding this comment.
Major: _ = get_data['x-medkit']['parameter']['value'] discards the reset value without asserting it. The comment says "should be reset to default (85.0)" but nothing verifies this. Replace with self.assertEqual(...).
| MIN_EXPECTED_APPS = 1 | ||
| REQUIRED_APPS = {'temp_sensor'} | ||
|
|
||
| def test_sse_stream_endpoint_returns_correct_headers(self): |
There was a problem hiding this comment.
Minor: If ReadTimeout fires before headers arrive, the except clause passes the test with zero header assertions.
| sub = self._create_subscription(interval='slow', duration=30) | ||
| events_url = f'http://localhost:8080{sub["event_source"]}' | ||
| self.addCleanup(self._delete_subscription, sub['id']) | ||
| events_url = f'http://localhost:{DEFAULT_PORT}{sub["event_source"]}' |
There was a problem hiding this comment.
Minor: SSE URLs hardcode http://localhost:{DEFAULT_PORT} (also at lines 334, 382). If port is overridden (as in auth/rate-limit tests), these point to the wrong server. Derive from self.BASE_URL.
| self.addCleanup( | ||
| lambda: requests.put( | ||
| f'{self.BASE_URL}/apps/temp_sensor/configurations/min_temp', | ||
| json={'data': 85.0}, |
There was a problem hiding this comment.
Minor: Cleanup resets min_temp via PUT to hardcoded 85.0. If default changes, cleanup silently sets the wrong value. Use DELETE instead (like the cleanup at line 171).
|
|
||
| @verifies REQ_INTEROP_003 | ||
| """ | ||
| app_id = self._find_app_only_id() |
There was a problem hiding this comment.
Minor: _find_app_only_id() called 4 times (lines 85, 116, 133, 150), making 8 identical HTTP requests. Cache the result in setUpClass.
| time.sleep(1) | ||
| return None | ||
|
|
||
| def create_execution(self, entity_endpoint, operation_id, *, input_data=None): |
There was a problem hiding this comment.
Minor: create_execution calls response.json() without checking status code. Non-JSON error response will raise unhelpful JSONDecodeError.
Pull Request
Summary
Refactor the gateway integration test suite into a dedicated
ros2_medkit_integration_testspackage with a shared Python test library (ros2_medkit_test_utils), two-tier test organization (features + scenarios), CMake labels, and structural fixes for flaky tests.What changed:
ros2_medkit_integration_tests- contains all integration tests, demo nodes, launch files, config YAMLs, and a shared Python test libraryros2_medkit_test_utils(4 modules) - eliminates ~1,200 lines of duplicated code:get_coverage_env()(9 copies), health polling (9 variants), demo node launch boilerplate, HTTP helperstest_integration.test.py(4,990 lines, 141 tests) split into 16 feature test files (~90 tests) and 10 scenario test files (~50 tests) - each file launches its own gateway with only the demo nodes it needsintegration;feature,integration;scenario) enablecolcon test --ctest-args -L featurefilteringKey library modules:
constants.py- shared API paths, ports, timeouts (replaces 50+ duplicated lines)coverage.py- singleget_coverage_env()(replaces 9 copies, ~270 lines)launch_helpers.py-create_test_launch()factory (replaces ~500 lines of inline launch boilerplate)gateway_test_case.py-GatewayTestCasebase class with health polling, HTTP helpers, discovery waiters, assertion helpersIssue
Type
Testing
Build verification:
Gateway unit tests (unchanged):
Integration test registration:
Run integration tests:
colcon test --packages-select ros2_medkit_integration_tests colcon test-result --verboseCoverage still works from separate package -
get_coverage_env()resolvesros2_medkit_gateway's build dir viaament_index_python, gateway binary writes.gcdatoGCOV_PREFIX, CIlcov --capture --directory buildscans the entire build directory.Issue #222 (flaky test_101): The race condition is resolved structurally -
test_scenario_action_lifecycle.test.pyreplaces the problematic test_100 + test_101 sequence. Each test creates its own execution (no concurrent action goals from previous tests).Issue #139 (multi-discovery-mode): Two new scenario files (
test_scenario_discovery_manifest.test.py,test_scenario_discovery_hybrid.test.py) test manifest-only and hybrid modes. Shared assertion helpers (assert_entity_exists,assert_entity_has_capabilities, etc.) inGatewayTestCasesupport mode-specific validation. Runtime mode is tested implicitly by all feature tests (default mode).Checklist